-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
[kotlin-spring][server] Feat: Return from controllers without ResponseEntity wrapper. #22377
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…upported by kotlin-spring generator
10c3d76 to
2f80415
Compare
| ) | ||
| fun addPet( @Valid @RequestBody pet: Pet): ResponseEntity<Pet> { | ||
| fun addPet( | ||
| @Valid @RequestBody pet: Pet |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am aware that this is not optimal (one would prefer to have this single param on one line with the opening and closing parentheses), but I think the trade-off is worth it for the multi-param methods
| fun updatePetWithForm( | ||
| @PathVariable("petId") petId: kotlin.Long, | ||
| @Valid @RequestParam(value = "name", required = false) name: kotlin.String? , | ||
| @Valid @RequestParam(value = "status", required = false) status: kotlin.String? | ||
| ): ResponseEntity<Unit> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that this is definitely an improvement over the current state of things. These signatures can get really long with all the annotations and then referencing the interface or controllers becomes quite cumbersome with a lot of horizontal scrolling.
…ty; fix double spaces, spaces before comma, incorrect spaces
| //for your own safety never directly reuse these path definitions in tests | ||
| const val PATH_ADD_PET: String = "/pet" | ||
| const val PATH_DELETE_PET: String = "/pet/{petId}" | ||
| const val PATH_FIND_PETS_BY_STATUS: String = "/pet/findByStatus" | ||
| const val PATH_FIND_PETS_BY_TAGS: String = "/pet/findByTags" | ||
| const val PATH_GET_PET_BY_ID: String = "/pet/{petId}" | ||
| const val PATH_UPDATE_PET: String = "/pet" | ||
| const val PATH_UPDATE_PET_WITH_FORM: String = "/pet/{petId}" | ||
| const val PATH_UPLOAD_FILE: String = "/pet/{petId}/uploadImage" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind this is that controller paths are generated as simple const val inside a companion object. Then in tests, the same paths can be manually redefined in a centralized utility class (often combined with a base path) to maintain explicit control and allow efficient DRY use across controller and integration tests. A unit test then asserts that each test path equals the corresponding generated path (or basePath + generatedPath), ensuring any divergence between controller and test definitions is caught while still allowing safe refactoring.
So that's why I would like to expose the paths like this.
Adds a new generator option
useResponseEntity(defaulttrue) for kotlin-spring (identical option exists in java-spring). When disabled, controller methods return the model type directly instead of wrapping inResponseEntity<>, and@ResponseStatusis added to preserve the HTTP status. (fixes #21833)Also replaces the (very recently introduced by me) option
declarativeInterfaceWrapResponseswhich is thus removed as theuseResponseEntitycan now control both and it does not make sense to keep both in place. However functionality depending on that option has not been yet released in any version other thanSNAPSHOT.Changes:
useResponseEntityconfig option. (defaulttrue)Motivation:
Provides flexibility for teams preferring direct model returns, aligning kotlin-spring with Java Spring generator capabilities and simplifying generated controller/interface signatures.
Breaking changes
This causes no breaking changes as the default behavior is unchanged. The only minor breaking change is the removal of
declarativeInterfaceWrapResponsesoption. The behavior controlled by this option is now also tied to the newuseResponseEntityoption.PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)"fixes #123"present in the PR description)